Skip to content

Conversation

vytas7
Copy link

@vytas7 vytas7 commented May 12, 2025

This is an incomplete workaround for #3512 (which "works for me" though for all practical purposes) just to get the ball rolling.

See the discussion on the issue (#3512) for the detailed explanation what this workaround does.

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a test and a changelog here 👍

@vytas7
Copy link
Author

vytas7 commented May 13, 2025

Thanks for taking a look at this @gaborbernat!

The question remains though what we should do with #3512? Should we leave it open?

What would the correct fix be for the case of the builder's root actually being reconfigured to another path, and in which scenario can this happen?

@vytas7
Copy link
Author

vytas7 commented May 13, 2025

OK, I'll link the newsfragment to this PR, leaving the issue as-is for now.

Copy link
Author

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaborbernat I need help on the tests.
I tried to transplant my MRE from the issue into the test suite, and it seems to work just fine, however, if I deliberately reintroduce the bug, it still passes. 🤔

I verified that a new backend executor is created in that case, however, that doesn't seem to cause any issues, unlike in the real run.

"""
sdist = pkg_with_pdm_backend / "dist" / "skeleton-0.1.1337.tar.gz"
proj = tox_project({"tox.ini": tox_ini}, base=pkg_with_pdm_backend)
result = proj.run("--installpkg", str(sdist), "--exit-and-dump-after", "10")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This --exit-and-dump-after should be increased to something larger (like 120) once I iron out the tests.

@vytas7 vytas7 requested a review from gaborbernat May 15, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants